-
Notifications
You must be signed in to change notification settings - Fork 34
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[ENH, WIP] Add multivariate connectivity methods #138
Conversation
DONE:
IN PROGRESS:
|
Hi @tsbinns how is this part going? Would you like me to review anything? Thanks! |
Hi @adam2392, this is still a WIP, however from Monday I have 2 full weeks to dedicate to this, so it will be finished then. I will tag you once I have it and all tests are passing.
I would be interested to hear your thoughts on the examples I added ( |
commit 44e21fe Author: Thomas Samuel Binns <t.s.binns@outlook.com> Date: Fri Jun 30 14:43:29 2023 +0200 updated GC example commit 648bb13 Author: Thomas Samuel Binns <t.s.binns@outlook.com> Date: Fri Jun 30 13:59:53 2023 +0200 updated docs commit 21a61a1 Author: Thomas Samuel Binns <t.s.binns@outlook.com> Date: Fri Jun 30 13:22:25 2023 +0200 updated docs commit 767719a Author: Thomas Samuel Binns <t.s.binns@outlook.com> Date: Fri Jun 30 13:03:41 2023 +0200 bug fix patterns indexing commit cd06739 Author: Thomas Samuel Binns <t.s.binns@outlook.com> Date: Fri Jun 30 12:51:20 2023 +0200 updated tests commit 13335f8 Author: Thomas Samuel Binns <t.s.binns@outlook.com> Date: Fri Jun 30 12:05:08 2023 +0200 updated tests commit 962a1ac Author: Thomas Samuel Binns <t.s.binns@outlook.com> Date: Thu Jun 29 16:49:53 2023 +0200 bug fixes commit 35f8b1f Author: Thomas Samuel Binns <t.s.binns@outlook.com> Date: Thu Jun 29 16:02:34 2023 +0200 bug fix patterns dims commit 754dfc7 Author: Thomas Samuel Binns <t.s.binns@outlook.com> Date: Thu Jun 29 15:38:54 2023 +0200 bug fixes commit 9098b2e Author: Thomas Samuel Binns <t.s.binns@outlook.com> Date: Thu Jun 29 15:38:41 2023 +0200 updated tests commit 9dbfa3b Author: Thomas Samuel Binns <t.s.binns@outlook.com> Date: Tue Jun 27 17:55:04 2023 +0200 removed redundant rank errors commit 42d7097 Author: Thomas Samuel Binns <t.s.binns@outlook.com> Date: Tue Jun 27 17:50:58 2023 +0200 updated examples commit 2a8b711 Author: Thomas Samuel Binns <t.s.binns@outlook.com> Date: Tue Jun 27 17:44:05 2023 +0200 updated time tests commit 0c4d668 Author: Thomas Samuel Binns <t.s.binns@outlook.com> Date: Tue Jun 27 17:43:43 2023 +0200 update mvc implementation commit 9f3d533 Author: Thomas Samuel Binns <t.s.binns@outlook.com> Date: Tue Jun 27 17:42:46 2023 +0200 bug fix rank estimation commit 213e586 Author: Thomas Samuel Binns <t.s.binns@outlook.com> Date: Tue Jun 27 17:42:32 2023 +0200 bug fix results storage commit aa4598a Author: Thomas Samuel Binns <t.s.binns@outlook.com> Date: Tue Jun 27 17:41:48 2023 +0200 bug fix mic edge case commit 1d8250e Author: Thomas Samuel Binns <t.s.binns@outlook.com> Date: Tue Jun 27 17:41:12 2023 +0200 updated patterns format commit e018586 Author: Thomas Samuel Binns <t.s.binns@outlook.com> Date: Tue Jun 27 17:26:18 2023 +0200 bug fix get_data commit 3f9062e Author: Thomas Samuel Binns <t.s.binns@outlook.com> Date: Mon Jun 26 18:54:39 2023 +0200 added mvc methods commit f2e4dad Author: Thomas Samuel Binns <t.s.binns@outlook.com> Date: Mon Jun 26 18:54:22 2023 +0200 bug fix mic commit 2778693 Author: Thomas Samuel Binns <t.s.binns@outlook.com> Date: Mon Jun 26 18:54:10 2023 +0200 updated examples
Hi @adam2392, I have now:
In terms of the goals of this PR I believe that is everything finished, assuming it is up to standard. I would be very grateful for any feedback! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @tsbinns sorry for the delay!
I left some notes. This is looking good imo! The examples are a nice touch because for someone that doesn't understand what these methods were previously, I feel like I'm learning something.
The main nits I have are unit-test coverage. You can just add separate unit-tests to achieve these. Other things I reviewed are just nits on explaining a concept and documentation.
I think once these are addressed, maybe a few more small changes. Overall though the high-level is fine w/ me and once it's in the hands of users for v0.6, we'll see how it goes :)
mne_connectivity/spectral/tests/data/example_multivariate_data.pkl
Outdated
Show resolved
Hide resolved
Thanks very much for the feedback @adam2392! I believe I have addressed everything, but please unresolve any comments you think I should work on further. Also please let me know when you think it's appropriate for me to update the Cheers! |
@tsbinns feel free to add a changelog entry rn that summarizes this major feature effort! Follow the pattern there to link your name/github page. I will take a look later in-depth on the code. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this LGTM once some final nits on grammar is fixed.
LGTM. Thanks @tsbinns and team! |
congrats on getting this one merged in! great effort. |
* add multivariate con base class * add multivariate imcoh classes * add multivar methods --------- Author: Thomas Samuel Binns <t.s.binns@outlook.com> Co-authored-by: Adam Li <adam2392@gmail.com>
@adam2392 Sorry for the delay, but I finally have something I think we can move forward with.
What I have implemented?
spectral_connectivity_epochs
function, with the original structure of e.g. theindices
parameter (i.e. no support for ragged indices added yet, as we discussed).spectral_connectivity_epochs
to reflect these new measures.rank
,n_lags
, andpatterns
).What are the limitations?
spectral_connectivity_epochs
function making assumptions about the number of signals in the seeds/targets matching the number of connections, which is not the case for the multivariate methods at this time. It is quite easy (and clean) to adapt the functions to handle this differently when only multivariate methods are called, however things become much trickier when trying to work with bivariate and multivariate methods simultaneously. If you would like the ability to compute bivariate and multivariate methods simultaneously to be included, perhaps we can adapt some of the functions to make this easier, otherwise we could stick with the current limitation I have imposed.indices
.indices
be ragged.What have I not added?
spectral_connectivity_time
. Again, this is still on my to-do list. I have structured the MIC/MIM and GC computation code in a way that we could very easily re-use the code I have added here and just switch out the time dimension (used in case of time-frequency modes) to store epoch information, so I do not consider this a major hurdle.There is, however, a bigger issue: the assertion of the connectivity object in
test_spectral_connectivity_parallel
matching that which has been saved and then reloaded now fails, because there is ~1 kB size difference in the objects. As far as I can tell, all of the critical information in the objects are identical (e.g. the results are the same, all entries inattrs
are the same). Interestingly, this only occurs for the multitaper and fourier modes. I checked in the original test, and the size of the saved and reloaded connectivity object is also not a perfect match, however it seems to be rounded to the same "~XX kB". My hunch would be that the attrs I added to the connectivity classes (rank
,n_lags
, andpatterns
), even when unfilled, are pushing the estimated size in the repr to be rounded up when combined with the pre-existing small difference in size that occurs when saving and reloading a connectivity object. Even though I think everything is in order, I am clearly not happy with this test suddenly failing, so if you have any ideas as to why this is happening, I would really appreciate it!The test in question:
https://github.com/tsbinns/mne-connectivity/blob/100e235c8520664510401d73394df1c953a6721f/mne_connectivity/spectral/tests/test_spectral.py#L134-L145
An example of the reprs from the test with the multitaper mode:
What are the next steps?
Adding further unit tests, as well as implementing these measures in
spectral_connectivity_time
are my two biggest targets. Once I have your input, I am happy to also start addressing the limitations I listed (e.g. by adding support for ragged indices). Making sure all tests are passing would also be nice!One again, I am very sorry for the delay (a lot of other work got in the way), but I am still very excited to move forward with this! If there is anything you think would be best discussed over a call, I am very happy to do that again. I will also answer anything here as soon as I can.
Cheers,
Thomas